Skip to content

fix #10997: check accessible from sealed parent not companion #11039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Jan 8, 2021

fixes #10997

given the commit 275a0a9 I am unclear why the accessibilty test in whyNotGenericSum needs to check for the companion.

I believe that it was a lucky coincidence that whyNotGenericSum passes for traits without a companion, i.e. if you trace the owners of any definition up to the root package then eventually you get the owner NoSymbol, which then succeeds in the case there is no companion, which breaks the criterion "all of its children are addressable through a path from its companion object".

The motivation for the change in this PR is that we use whyNotGenericSum to implement isGenericSum. Then isGenericSum is used by Synthesizer.sumMirror to decide if something gets a synthesized Mirror.Sum.

Since Synthesizer.sumMirror still generates a mirror even if there is no companion, I can not see a reason to fail whyNotGenericSum if there is no companion.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the fromProduct method? That is not generated if there is no companion, right? How does that manifest itself?

We probably should have a test involving fromProduct

@odersky odersky assigned bishabosha and unassigned odersky Jan 11, 2021
@bishabosha
Copy link
Member Author

What about the fromProduct method? That is not generated if there is no companion, right? How does that manifest itself?

We probably should have a test involving fromProduct

I have added a test using ordinal, and fromProduct but there should be no change in behaviour for fromProduct as it is only generated on Mirror.Product, which is not affected by this change.

@bishabosha
Copy link
Member Author

bishabosha commented Jan 11, 2021

Edit: I have added a test case to prevent a crash in the following:

class ClassWrapper {
  sealed trait Parent
  case class Foo(x: Int, y: Int, s: String) extends Parent
}

@main def Test =
  val cw = new ClassWrapper()
  val mirrorParent = summon[deriving.Mirror.Of[cw.Parent]] // error - dont synthesize mirror in this case

If we allow this case then the generated mirror will use paths such as ClassWrapper.this.Foo in def Test which is an illegal scope.

A temporary mitigation is included in the next commit to check accessibility from the scope of the generated mirror, a follow up PR could correct the paths generated when the mirror has a class instance in its path

@bishabosha
Copy link
Member Author

I have added a test to check that the mirror can also access the children if it is an anonymous class

@bishabosha bishabosha requested a review from odersky January 11, 2021 13:06
@bishabosha bishabosha assigned odersky and unassigned bishabosha Jan 11, 2021
@bishabosha bishabosha force-pushed the fix-10997 branch 3 times, most recently from 60d8038 to 6d1e7fa Compare January 11, 2021 14:42
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the downside of not having a companion is that everytime you demand a mirror a new object is created, right? Which means a new class is generated per call site, and an object per dynamic call.

I think that's a serious degradation that is not at all obvious. So at least we should have a warning that this happens and an encouragement to create a companion object.

Typeclass derivation libraries often need to be high-performance, so we should make users aware of the trap.

@bishabosha
Copy link
Member Author

I think that's a serious degradation that is not at all obvious. So at least we should have a warning that this happens and an encouragement to create a companion object.

Should this warning be for all anonymous mirrors (i.e. including mirrors for Scala 2 types) or just the ones for types in Scala 3.

I would assume probably avoid noise for Scala 2 types

@bishabosha bishabosha requested a review from odersky January 12, 2021 17:19
@bishabosha bishabosha assigned odersky and unassigned bishabosha Jan 12, 2021
@bishabosha bishabosha force-pushed the fix-10997 branch 4 times, most recently from 646f236 to 75ab830 Compare January 13, 2021 00:52
@bishabosha
Copy link
Member Author

adding the warning is breaking shapeless though with an unrelated error

@bishabosha bishabosha assigned bishabosha and unassigned odersky Jan 13, 2021
@odersky
Copy link
Contributor

odersky commented Jan 13, 2021

I think I misunderstood two cases. If a trait has a derived clause

sealed trait T derives C

it gets a companion automatically. That's the case I was after. I thought somehow that we would then fallback to anonymous classes, but that's obviously not the case. If we summon a mirror outside, we should be prepared that an anonymous class is created. So I think we can just revert the last commit and drop the warning. Sorry for the confusion!

@bishabosha
Copy link
Member Author

I have reverted the changes to add the warning

@bishabosha
Copy link
Member Author

bishabosha commented Jan 13, 2021

the problem in shapeless was that it had fatal warnings turned on which causes an error in the macro to summon a shapeless.typeable.Typeable for a sealed trait with no companion, and so suppressed the error for summoning an anonymous Mirror in favour of displaying the Implicit not found error. If fatal warnings is turned off, you instead see the warning about an anonymous mirror but the implicit Typeable can still be found

@bishabosha bishabosha merged commit 3a9f122 into scala:master Jan 13, 2021
@bishabosha bishabosha deleted the fix-10997 branch January 13, 2021 13:45
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mirror.Sum not synthesised for base class without companion
3 participants